Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up Api Error and indicate future status clearly #424

Merged
merged 2 commits into from
Jan 9, 2023
Merged

Clean up Api Error and indicate future status clearly #424

merged 2 commits into from
Jan 9, 2023
+91 −58

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Jan 6, 2023 •
edited
Loading

  • removes unused Api Error
  • use clearer naming for the Api Error
  • UnexptedTxStatus Error contains a clearly defined Enum, not a String. This should be easier to check for a specific status (e.g. Future), than with the previously used string.
  • Removed FinalityTimeout from expected. Reasoning: If one waits for Finalized, but a Timeout is encountered, the loop would go on forever.

@haerdib haerdib self-assigned this Jan 6, 2023
@haerdib haerdib changed the title Indicate future status clearly Clean up Api Error and indicate future status clearly Jan 6, 2023
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E2-breaksapi labels Jan 6, 2023
haerdib
haerdib commented Jan 6, 2023
View reviewed changes
node-api/src/lib.rs
@@ -21,7 +21,6 @@ use alloc::{borrow::ToOwned, vec::Vec};
use codec::{Decode, Encode};

pub use decoder::*;
pub use error::*;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to remove this one because in the api the node-api is also rexported with pub use ..::*; The default error should be the one from api-client, not the node-api.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, in general I think we should be more conscious about exports anyhow than just wildcard re-exporting anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. Let me take that into an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#430 :) Thanks for the input

src/api/error.rs
@@ -32,13 +32,13 @@ pub enum Error {
Metadata(MetadataError),
InvalidMetadata(InvalidMetadataError),
NodeApi(ac_node_api::error::Error),
StorageValueDecode(codec::Error),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to Codec, as it's already used for general decoding errors, not just StorageValue.

src/api/error.rs
@@ -32,13 +32,13 @@ pub enum Error {
Metadata(MetadataError),
InvalidMetadata(InvalidMetadataError),
NodeApi(ac_node_api::error::Error),
StorageValueDecode(codec::Error),
UnsupportedXtStatus(XtStatus),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, as it was not used at all.

@haerdib haerdib marked this pull request as ready for review January 6, 2023 09:47
@haerdib haerdib force-pushed the bh/indicate-future-status-clearly branch from 23b2564 to 35396fa Compare January 9, 2023 08:28
Niederb
Niederb approved these changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@Niederb Niederb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I like the improved descriptions and the new UnexpectedTxStatus

@haerdib
remove finalitytimeout from is_valid
72809e4 
rename is_supported to is_valid

rename is_valid to is_expected

some more clean up

add unexpected Tx status enum

add some comment
@haerdib haerdib force-pushed the bh/indicate-future-status-clearly branch from 35396fa to 72809e4 Compare January 9, 2023 09:02
clangenb
clangenb reviewed Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me!

node-api/src/lib.rs
@@ -21,7 +21,6 @@ use alloc::{borrow::ToOwned, vec::Vec};
use codec::{Decode, Encode};

pub use decoder::*;
pub use error::*;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, in general I think we should be more conscious about exports anyhow than just wildcard re-exporting anything.

@haerdib haerdib mentioned this pull request Jan 9, 2023
Closed
@haerdib haerdib merged commit bf42f66 into master Jan 9, 2023
@haerdib haerdib deleted the bh/indicate-future-status-clearly branch January 9, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clangenb clangenb clangenb left review comments

@Niederb Niederb Niederb approved these changes

Assignees

@haerdib haerdib

Labels
E2-breaksapi F7-enhancement Enhances an already existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@haerdib @Niederb @clangenb